Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix hudi connector gets stuck #19506 #20027

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

willzgw
Copy link
Contributor

@willzgw willzgw commented Dec 5, 2023

Description

-Fix hudi connector gets stuck

Additional context and related issues

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix: Trino Hudi connector gets stuck while attempting to read empty partitions of a partitioned Hudi table. ({issue}`19506 `)

@cla-bot cla-bot bot added the cla-signed label Dec 5, 2023
@github-actions github-actions bot added the hudi Hudi connector label Dec 5, 2023
Copy link
Member

@ebyhr ebyhr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test?

Copy link
Member

@electrum electrum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to add a test for this?

@willzgw
Copy link
Contributor Author

willzgw commented Dec 13, 2023

Hi @electrum
I'm not quite sure what to do when we get an empty partition, ignore it or throw an exception?

@uroell
Copy link

uroell commented Jan 3, 2024

Hi @electrum I'm not quite sure what to do when we get an empty partition, ignore it or throw an exception?

Hi @willzgw, thank you very much for your contribution! This helps a lot. Just a suggestion for your question: In the hive connector there is this property called "hive.ignore-absent-partitions" with default "false" to switch the behavior. Maybe you could implement the same for the hudi connector?

@codesorcery
Copy link
Contributor

codesorcery commented Jan 23, 2024

#20151 fixes the underlying problem of Trino failing on empty Hudi partitions. So when this PR gets merged, there should be no reason to handle it here.
So it should be enough to reduce the scope of this PR to something like

--- a/plugin/trino-hudi/src/main/java/io/trino/plugin/hudi/split/HudiBackgroundSplitLoader.java
+++ b/plugin/trino-hudi/src/main/java/io/trino/plugin/hudi/split/HudiBackgroundSplitLoader.java
@@ -14,6 +14,7 @@
 package io.trino.plugin.hudi.split;
 
 import com.google.common.util.concurrent.Futures;
+import com.google.common.util.concurrent.ListenableFuture;
 import io.trino.plugin.hive.util.AsyncQueue;
 import io.trino.plugin.hudi.HudiTableHandle;
 import io.trino.plugin.hudi.partition.HudiPartitionInfoLoader;
@@ -28,8 +29,8 @@ import java.util.List;
 import java.util.concurrent.ConcurrentLinkedDeque;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Executor;
-import java.util.concurrent.Future;
 
+import static com.google.common.util.concurrent.MoreExecutors.directExecutor;
 import static io.trino.plugin.hudi.HudiErrorCode.HUDI_CANNOT_OPEN_SPLIT;
 import static io.trino.plugin.hudi.HudiSessionProperties.getSplitGeneratorParallelism;
 import static java.util.Objects.requireNonNull;
@@ -66,7 +67,7 @@ public class HudiBackgroundSplitLoader
     {
         Deque<String> partitionQueue = new ConcurrentLinkedDeque<>(partitions);
         List<HudiPartitionInfoLoader> splitGeneratorList = new ArrayList<>();
-        List<Future> splitGeneratorFutures = new ArrayList<>();
+        List<ListenableFuture<Void>> splitGeneratorFutures = new ArrayList<>();
 
         // Start a number of partition split generators to generate the splits in parallel
         for (int i = 0; i < splitGeneratorNumThreads; i++) {
@@ -79,16 +80,18 @@ public class HudiBackgroundSplitLoader
             // Let the split generator stop once the partition queue is empty
             generator.stopRunning();
         }
-
-        // Wait for all split generators to finish
-        for (Future future : splitGeneratorFutures) {
-            try {
-                future.get();
-            }
-            catch (InterruptedException | ExecutionException e) {
-                throw new TrinoException(HUDI_CANNOT_OPEN_SPLIT, "Error generating Hudi split", e);
-            }
+        try {
+            // Wait for all split generators to finish
+            Futures.whenAllComplete(splitGeneratorFutures) // also succeeds when some tasks fail
+                    .run(asyncQueue::finish, directExecutor())
+                    .get(); // will throw an ExecutionException when one of the tasks failed
+        }
+        catch (ExecutionException e) {
+            throw new TrinoException(HUDI_CANNOT_OPEN_SPLIT, "Error generating Hudi split", e);
+        }
+        catch (InterruptedException e) {
+            Thread.currentThread().interrupt();
+            throw new TrinoException(HUDI_CANNOT_OPEN_SPLIT, "Error generating Hudi split", e);
         }
-        asyncQueue.finish();
     }
 }

@willzgw
Copy link
Contributor Author

willzgw commented Jan 25, 2024

Hi @ebyhr @electrum
I tried to add a test to TestHudiSmokeTest but Trino will throw exception when creating table due to partition not exists.
It seems not convenient to add test cases.
Do you have any suggestions?

Copy link

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

@github-actions github-actions bot added the stale label Feb 15, 2024
@wendigo wendigo requested a review from ebyhr February 16, 2024 17:11
@github-actions github-actions bot removed the stale label Feb 16, 2024
Copy link

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

@ebyhr
Copy link
Member

ebyhr commented Mar 14, 2024

I tried to add a test to TestHudiSmokeTest but Trino will throw exception when creating table due to partition not exists.

@willzgw Sorry for my late reply. We could create the table on Spark and add the contents under resources directory. You can refer to resources/hudi-testing-data/hudi_cow_pt_tbl.

@willzgw
Copy link
Contributor Author

willzgw commented Mar 14, 2024

I tried to add a test to TestHudiSmokeTest but Trino will throw exception when creating table due to partition not exists.

@willzgw Sorry for my late reply. We could create the table on Spark and add the contents under resources directory. You can refer to resources/hudi-testing-data/hudi_cow_pt_tbl.

Hi @ebyhr
Thanks for your advice.
I've actually tried this approach before. We want to test the case where the Partition does not exist, which requires the metadata to contain files that do not actually exist (in resource). However, it will report an error by prompting that the corresponding file is missing from the Resource.

Copy link

github-actions bot commented Apr 4, 2024

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

@github-actions github-actions bot added the stale label Apr 4, 2024
@mosabua
Copy link
Member

mosabua commented Apr 6, 2024

Should we merge this PR given that you approved @electrum ?

Copy link
Contributor

@codope codope left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me. One thing is we also need to update the huddi connector page for the new config introuced by this PR. I am ok if it's done in this PR or as a followup. Also, please rebase.

@codesorcery
Copy link
Contributor

Do I see it right, that one then has to explicitly set hudi.ignore-absent-partitions=false to get the current behavior? IMHO, that should then definitely be highlighted in the release notes, since it's a breaking change.

Copy link

github-actions bot commented May 2, 2024

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

@github-actions github-actions bot added the stale label May 2, 2024
Copy link

Closing this pull request, as it has been stale for six weeks. Feel free to re-open at any time.

@github-actions github-actions bot closed this May 24, 2024
@colebow colebow reopened this May 24, 2024
@colebow colebow added the stale-ignore Use this label on PRs that should be ignored by the stale bot so they are not flagged or closed. label May 24, 2024
Copy link
Contributor

@chenjian2664 chenjian2664 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rearrange the commits and squash to two, one is fixing the stuck issue, another is adding the session property.

@mosabua
Copy link
Member

mosabua commented Aug 26, 2024

@electrum can we merge this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed docs hudi Hudi connector stale stale-ignore Use this label on PRs that should be ignored by the stale bot so they are not flagged or closed.
Development

Successfully merging this pull request may close these issues.

9 participants